Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore status fields during drift detection #2522

Merged
merged 5 commits into from
Jun 20, 2024
Merged

Conversation

aruiz14
Copy link
Contributor

@aruiz14 aruiz14 commented Jun 14, 2024

Refers to #2521

Use a new StatusNormalizer when comparing deployed resources against the desired spec, since status is normally not part of it.

@aruiz14 aruiz14 requested a review from a team as a code owner June 14, 2024 12:43
@aruiz14 aruiz14 force-pushed the ignore-status-diff branch 3 times, most recently from 03f433c to 2fbbac3 Compare June 17, 2024 10:34
weyfonk
weyfonk previously approved these changes Jun 17, 2024
Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix :)

e2e/drift/drift_test.go Outdated Show resolved Hide resolved
@aruiz14 aruiz14 force-pushed the ignore-status-diff branch from 3fa491a to e985455 Compare June 19, 2024 07:05
@aruiz14 aruiz14 closed this Jun 20, 2024
@aruiz14 aruiz14 deleted the ignore-status-diff branch June 20, 2024 08:04
@aruiz14 aruiz14 restored the ignore-status-diff branch June 20, 2024 08:05
@aruiz14 aruiz14 reopened this Jun 20, 2024
@aruiz14 aruiz14 force-pushed the ignore-status-diff branch from e985455 to 79b73ec Compare June 20, 2024 08:05
Comment on lines 188 to 196
Context("Resource manifests containing status fields", func() {
It("Is marked as not ready", func() {
bundleName := "drift-correction-test-drift-ignore-status"
Eventually(func() bool {
b := getBundle(bundleName, k)
return b.Status.Summary.Ready == 1
}).Should(BeTrue())
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with an older version, and checked that it fails. The bundle never reaches a ready status:

❯ kubectl get bundle -n fleet-local -w drift-correction-test-drift-ignore-status
NAME                                        BUNDLEDEPLOYMENTS-READY   STATUS
drift-correction-test-drift-ignore-status   0/1                       Modified(1) [Cluster fleet-local/local]; deployment.apps drift-ignore-status/nginx-deployment modified {"status":{"collisionCount":0}}

@aruiz14 aruiz14 force-pushed the ignore-status-diff branch from ace0807 to 0478e62 Compare June 20, 2024 10:26
bigkevmcd
bigkevmcd previously approved these changes Jun 20, 2024
weyfonk
weyfonk previously approved these changes Jun 20, 2024
Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@aruiz14 aruiz14 changed the title Remove status fields before drift detection Ignore status fields during drift detection Jun 20, 2024
@aruiz14 aruiz14 enabled auto-merge (squash) June 20, 2024 16:10
@aruiz14 aruiz14 merged commit 6fa6a0a into main Jun 20, 2024
8 checks passed
@aruiz14 aruiz14 deleted the ignore-status-diff branch June 20, 2024 16:52
Tommy12789 pushed a commit to Tommy12789/fleet that referenced this pull request Jun 26, 2024
Tommy12789 pushed a commit to Tommy12789/fleet that referenced this pull request Jun 26, 2024
Tommy12789 pushed a commit to Tommy12789/fleet that referenced this pull request Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants